vfs: integrate with CJS and ESM module loaders#63653
Conversation
|
Review requested:
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #63653 +/- ##
==========================================
- Coverage 92.04% 90.10% -1.95%
==========================================
Files 381 741 +360
Lines 169208 242313 +73105
Branches 25926 45732 +19806
==========================================
+ Hits 155746 218325 +62579
- Misses 13174 15467 +2293
- Partials 288 8521 +8233
🚀 New features to boost your workflow:
|
|
@joyeecheung take a look, should be easier to review. |
Restore the "DO NOT depend on the patchability" warnings in esm/load.js and esm/resolve.js that were dropped along with the fs imports. The warning still applies; it now also points at node:vfs as one of the formal hook mechanisms callers should reach for instead. Addresses review feedback from @jsumners-nr in nodejs#63653
6321e08 to
51b033a
Compare
joyeecheung
left a comment
There was a problem hiding this comment.
A design question recently occurred to me: have we explored the versioning of the mounting?
what do you mean? You mean multiple vfs layers on top of each other? |
For the stacks to have some kind of version number/ID to identify the current status? BTW I just noticed that there's no mention of |
I did purge them when doing the splitting; I forgot to bring them back. I'll add them to this PR. |
No but we totally should. |
Each VirtualFileSystem now exposes a per-process monotonically increasing `layerId`, assigned at construction. The id is stable across mount/unmount cycles for the lifetime of the instance and surfaces in: - debug() output for register / deregister so the layer stack is visible when NODE_DEBUG=vfs is enabled; - the overlap ERR_INVALID_STATE message, which now names the layer ids of the conflicting mounts. The id is the building block for tagging cache entries with the owning VFS, which a follow-up will use to replace the global loader-cache flush in deregisterVFS with a scoped purge. Refs: nodejs#63653 Signed-off-by: Matteo Collina <hello@matteocollina.com>
51b033a to
294a19c
Compare
Replace the global loader-cache flush in deregisterVFS with a scope-purge that only drops entries owned by the unmounting VFS. Per-layer ownership is determined two ways: - For CJS-style filename-keyed caches (Module._cache, Module._pathCache, the CJS stat cache, the helpers.js realpath cache, and the package.json caches) entries are filtered with `vfs.shouldHandle(filename)`. __filename stays a clean absolute path so user code that does `path.dirname(__filename)` or similar is unaffected. - For the ESM cascaded loader's loadCache, entries are tagged at resolve time: when finalizeResolution() detects the resolved path is VFS-owned (via the new loaderGetLayerForPath hook), it appends `?vfs-layer=<id>` to the URL. The tag surfaces in `import.meta.url`, matching the cache-busting pattern used by HMR tooling. On deregister, entries whose URL carries the tag for the unmounting layer are deleted. Multi-mount setups no longer pay the cross-VFS cache-warmup penalty when a single VFS unmounts, and ESM modules loaded from a VFS become reachable for purge instead of leaking forever in the cascaded loader. New helpers exposed for VFS: - cjs/loader.js: clearStatCacheForVFS - helpers.js: purgeRealpathCacheForVFS, loaderGetLayerForPath - package_json_reader.js: purgePackageJSONCacheForVFS Adds test-vfs-scoped-cache-purge covering both the multi-mount isolation and the import.meta.url tag visibility. Refs: nodejs#63653 Signed-off-by: Matteo Collina <hello@matteocollina.com>
|
@joyeecheung PTAL |
75bb5c7 to
99a5a5c
Compare
Signed-off-by: Matteo Collina <hello@matteocollina.com>
Signed-off-by: Matteo Collina <hello@matteocollina.com>
Signed-off-by: Matteo Collina <hello@matteocollina.com>
Signed-off-by: Matteo Collina <hello@matteocollina.com>
Signed-off-by: Matteo Collina <hello@matteocollina.com>
This reverts commit a95717a.
Signed-off-by: Matteo Collina <hello@matteocollina.com>
Signed-off-by: Matteo Collina <hello@matteocollina.com>
Signed-off-by: Matteo Collina <hello@matteocollina.com>
Signed-off-by: Matteo Collina <hello@matteocollina.com>
Signed-off-by: Matteo Collina <hello@matteocollina.com>
Signed-off-by: Matteo Collina <hello@matteocollina.com>
d1d72c3 to
0d5fb61
Compare
|
@joyeecheung done |
joyeecheung
left a comment
There was a problem hiding this comment.
Can you remove the (AI generated?) comments that just restate what happens in code with prose or stating how other code calls it currently (which can get out of sync very quickly and is anti-encapsulation?)
Also I think the documentation still doesn't clarify a core question: how is the module lookup algorithm altered once an VFS is mounted. It needs to specify that precisely in the module loading spec in modules.md instead of just leaving some hand wavy descriptions like "the module loader will consult it, it will work" (but how and where in the algorithm? and when will they NOT be consulted?)
| // Toggleable loader hooks for VFS support. Each `loader*` wrapper below | ||
| // funnels through a single factory: if an override is registered for the | ||
| // original function it is called first; anything but `undefined` short- | ||
| // circuits the wrapper, otherwise the wrapper falls through to the | ||
| // pinned native. This keeps the wrapper surface a one-liner per binding | ||
| // - adding a new binding is one `wrapLoaderMethod` call, no extra state | ||
| // variables or per-hook setter branches to keep in sync. |
There was a problem hiding this comment.
| // Toggleable loader hooks for VFS support. Each `loader*` wrapper below | |
| // funnels through a single factory: if an override is registered for the | |
| // original function it is called first; anything but `undefined` short- | |
| // circuits the wrapper, otherwise the wrapper falls through to the | |
| // pinned native. This keeps the wrapper surface a one-liner per binding | |
| // - adding a new binding is one `wrapLoaderMethod` call, no extra state | |
| // variables or per-hook setter branches to keep in sync. | |
| // Toggleable loader hooks for VFS support. |
This is just describing what obviously happens below with 7 lines of prose?
| // internal/vfs/setup.js) map directly into this array; indexes 0..6 | ||
| // use packageConfig.main as the prefix, 7..9 use pkgPath directly. |
There was a problem hiding this comment.
| // internal/vfs/setup.js) map directly into this array; indexes 0..6 | |
| // use packageConfig.main as the prefix, 7..9 use pkgPath directly. | |
| // internal/vfs/setup.js) map directly into this array. |
This is repeating what comments below says.
| participate in module resolution and loading. Both | ||
| `require()` / `require.resolve()` (CommonJS) and `import` / | ||
| `import.meta.resolve()` (ECMAScript modules) consult the VFS through | ||
| the same toggleable hooks that `node:fs` uses, so files served from | ||
| the VFS are first-class modules: `package.json` is honoured, | ||
| extensionless files are sniffed for Wasm vs. JavaScript, conditional | ||
| `exports` / `imports` work, and so on. |
There was a problem hiding this comment.
| participate in module resolution and loading. Both | |
| `require()` / `require.resolve()` (CommonJS) and `import` / | |
| `import.meta.resolve()` (ECMAScript modules) consult the VFS through | |
| the same toggleable hooks that `node:fs` uses, so files served from | |
| the VFS are first-class modules: `package.json` is honoured, | |
| extensionless files are sniffed for Wasm vs. JavaScript, conditional | |
| `exports` / `imports` work, and so on. | |
| participate in module resolution and loading. Both | |
| `require()` / `require.resolve()` (CommonJS) and `import` / | |
| `import.meta.resolve()` (ECMAScript modules) consult the VFS through | |
| the same toggleable hooks that `node:fs` uses, so files served from | |
| the VFS are first-class modules: `package.json` is honoured, | |
| extensionless files are sniffed for Wasm vs. JavaScript, conditional | |
| `exports` / `imports` work, and so on. |
This paragraph is listing example without actually clarifying anything - exactly how do they appear in the module lookup algorithm, and with what precedence?
Signed-off-by: Matteo Collina <hello@matteocollina.com>
Signed-off-by: Matteo Collina <hello@matteocollina.com>
Co-authored-by: Joyee Cheung <joyeec9h3@gmail.com>
The microbenchmark drove 1000+ iterations to reach the optimizing tier, which is not representative of real workloads. Module-loading against a mounted VFS is the meaningful signal and can be measured with the existing benchmark harness.
Measures loading a large CJS or ESM module graph from a mounted VFS, relying on the unmount cache purge so every iteration is a cold load.
|
@joyeecheung PTAL, I think I did all the fix you suggested. |
joyeecheung
left a comment
There was a problem hiding this comment.
I am not sure if it's the AI talking (looks like it is) but this is now changing/deleting comments of code that aren't changed in this PR...
| instead, every file system operation those algorithms perform | ||
| (existence probes, file reads, `package.json` lookups, real-path | ||
| resolution) is dispatched on the path being probed: paths under a |
There was a problem hiding this comment.
| instead, every file system operation those algorithms perform | |
| (existence probes, file reads, `package.json` lookups, real-path | |
| resolution) is dispatched on the path being probed: paths under a | |
| instead, every file system operation those algorithms perform is dispatched | |
| on the path being probed: paths under a |
These implementation details should not be enumerated in the docs.
| behave as first-class modules: `package.json` is honoured, | ||
| conditional [`"exports"`][] / [`"imports"`][] work, and so on. |
There was a problem hiding this comment.
| behave as first-class modules: `package.json` is honoured, | |
| conditional [`"exports"`][] / [`"imports"`][] work, and so on. | |
| behave as first-class modules. |
These look like random hand-wavy examples that seem to be meaningful but actually clarify nothing again - if we have to explain with an example it's better to provide a snippet etc. to make it more concrete.
| root: `package.json` scope lookups (for [`"type"`][], | ||
| [`"exports"`][], [`"imports"`][], and the CommonJS directory | ||
| [`"main"`][]) and [loading from `node_modules` folders][] stop at |
There was a problem hiding this comment.
| root: `package.json` scope lookups (for [`"type"`][], | |
| [`"exports"`][], [`"imports"`][], and the CommonJS directory | |
| [`"main"`][]) and [loading from `node_modules` folders][] stop at | |
| root: `package.json` scope lookups and [loading from `node_modules` folders][] stop at |
Again we should not enumerate them here. If we must, simply linking to the documentation in packages is better.
| [`"exports"`][], [`"imports"`][], and the CommonJS directory | ||
| [`"main"`][]) and [loading from `node_modules` folders][] stop at | ||
| the mount point instead of continuing into the real file system | ||
| ([the global folders][], such as `NODE_PATH`, are legacy CommonJS |
There was a problem hiding this comment.
The "instead of continuing into the real file system" part is leading users to spare a thought about a case that is not practical in the first place, because the mount point cannot actually have real files in the file system, no continuation can be performed even if Node.js wants to. I think it's clearer to simply list an example lookup, for example in /foo/bar/main.cjs, require('baz') should look up
- `$MOUNT_POINT/foo/bar/node_modules/baz`
- `$MOUNT_POINT/foo/node_modules/baz`
- `$MOUNT_POINT/node_modules/baz`
- If $NODE_PATH is specified, search folders listed in $NODE_PATH
- `$HOME/.node_modules/baz`
- `$HOME/.node_libraries/baz`
- `$PREFIX/lib/node/baz`
| return providerPath; | ||
| } | ||
|
|
||
| // ==================== FS Operations (Sync) ==================== |
There was a problem hiding this comment.
Why is it deleting JSDocs and other unrelated comments in this file?
| myVfs.unmount(); | ||
| ``` | ||
|
|
||
| For ECMAScript modules, convert mounted paths to `file:` URLs before |
There was a problem hiding this comment.
Non-blocking: I wonder if we should simply supply vfs.mountPointURL, then users don't have to convert it...another solution is to provide path methods like vfs.resolve/vfs.resolveAsURL, then users don't need to go with real path methods to build VFS paths. Also what actually happens under the URL conversion should be much simpler, because there won't be any poking (is it Windows? etc.), it's just simple concatenation.
| Module identity follows the path: `__filename` and `module.filename` | ||
| are the plain absolute path of the module under the mount point, and | ||
| `import.meta.url` is the corresponding `file:` URL, with no synthetic | ||
| decorations. Importing the same virtual path repeatedly, including | ||
| through `import.meta.resolve()`, yields the same module instance, | ||
| exactly as for real files. |
There was a problem hiding this comment.
| Module identity follows the path: `__filename` and `module.filename` | |
| are the plain absolute path of the module under the mount point, and | |
| `import.meta.url` is the corresponding `file:` URL, with no synthetic | |
| decorations. Importing the same virtual path repeatedly, including | |
| through `import.meta.resolve()`, yields the same module instance, | |
| exactly as for real files. | |
| CommonJS modules loaded from a mounted VFS are identified by their VFS paths | |
| that start with the mount point. This is reflected in, for example, `__filename` and | |
| `__dirname` in the module, or the errors stack traces involving functions from | |
| the VFS modules. ES modules in the VFS are similarly identified by the `file:` URL of | |
| their VFS paths and this is reflected in e.g. `import.meta.url`. | |
| Like modules loaded from the real file system, modules loaded from the VFS are | |
| cached on the first load. When `require()` or `import()` is used to load an absolute | |
| path or URL that falls under the mounted VFS multiple times, the module is only loaded | |
| once and subsequent calls return the same instance. |
The current description is mixing CommonJS and ESM. It's better to describe different cases more clearly.
| Mounting and unmounting do not invalidate ESM modules that are | ||
| already executing. As with any other module-system teardown, | ||
| unmounting a VFS while the import graph below it is still loading is | ||
| the caller's responsibility to avoid. |
There was a problem hiding this comment.
| Mounting and unmounting do not invalidate ESM modules that are | |
| already executing. As with any other module-system teardown, | |
| unmounting a VFS while the import graph below it is still loading is | |
| the caller's responsibility to avoid. | |
| Mounting and unmounting do not stop any module execution that is already started, | |
| or invalidate any objects materialized from VFS modules that are already executed. | |
| As with modules in the real file system, the callers are responsible of avoiding | |
| removal or invalidation of modules in the virtual file system while they are being loaded. |
You could technically unmount CJS while it is executing too (if you make the vfs object reference available e.g. with globals, then call unmount at the top level of a CJS module being loaded from that VFS).
| stat: (filename) => internalFsBinding.internalModuleStat(filename), | ||
| readFile: (filename, options) => fs.readFileSync(filename, options), | ||
| realpath: (requestPath) => fs.realpathSync(requestPath, { |
There was a problem hiding this comment.
| stat: (filename) => internalFsBinding.internalModuleStat(filename), | |
| readFile: (filename, options) => fs.readFileSync(filename, options), | |
| realpath: (requestPath) => fs.realpathSync(requestPath, { | |
| internalModuleStat: (filename) => internalFsBinding.internalModuleStat(filename), | |
| readFileSync: (filename, options) => fs.readFileSync(filename, options), | |
| realpathSync: (requestPath) => fs.realpathSync(requestPath, { |
we should use exact names to avoid ambiguity with other methods that actually use that name
| }; | ||
| } | ||
|
|
||
| const loaderStat = wrapLoaderMethod(nativeLoaderMethods.stat); |
There was a problem hiding this comment.
Can we not just do something like
const wrappers = { __proto__: null };
const loaderMethodKeys = ObjectKeys(nativeLoaderMethods);
for (let i = 0; i < loaderMethodKeys.length; ++i) {
const key = loaderMethodKeys[i];
wrappers[key] = nativeLoaderMethods[key];
}Then we can simply use the key to identify overrides, and setLoaderFsOverrides and setLoaderPackageOverrides can be merged into
setLoaderOverrides(overrides) {
for (let i = 0; i < loaderMethodKeys.length; ++i) {
const key = loaderMethodKeys[i];
if (overrides[key]) {
loaderOverrides.set(key, override);
} else {
loaderOverrides.delete(key);
}
}
hasLoaderOverrides = loaderOverrides.size > 0;
}And then we just exports the wrappers to other modules instead of enumerating them all in the export list below. This means in the future if anything is moved to C++ that leads to an overridable point, we no longer have to add like 4 lines of boilerplate below to hook it up, simply adding a key-value pair in nativeLoaderMethods is enough and everything else is automatic.
Makes
require()andimportresolve files served bynode:vfs. Before this PR, mounted VFS files were only visible throughfs.*; the loaders went straight to the real filesystem.Design
vfs.mount()takes no arguments and returns the reserved absolute mount point of the instance:${os.devNull}/vfs/<layerId>(for example/dev/null/vfs/0).os.devNullis a character device on POSIX and a device-namespace path on Windows; neither can have child filesystem entries, so no real path can ever exist under this root.Everything about ownership is decidable from the path alone:
benchmark/vfs/bench-fs-dispatch.js) reports flat per-call latency across 1..10 mounted layers.realpathSyncof a VFS entry always resolves to another path under the same mount point, so cache entries hidden behind symlinks are captured by the prefix scan.file:URLs under the mount point;import(import.meta.resolve(x))re-hits the same module job.Two instances mounting simultaneously never collide (each gets its own
layer-<id>segment), so there is no overlap validation and no ordering hazard between mounts.Loader integration
Toggleable wrappers in the loaders. Null fast-path when no VFS is mounted; otherwise the VFS answers
stat/readFile/realpath/legacyMainResolve/getFormatOfExtensionlessFileand the fourpackage.jsonC++-binding calls.Module identity follows the path:
__filename,module.filename, andimport.meta.urlare the plain absolute path (orfile:URL) of the module under the mount point — no synthetic decorations.Review guide
Suggested reading order:
lib/internal/vfs/router.jsgetVfsRoot,getLayerRoot,getLayerIdFromPath.lib/internal/vfs/file_system.jsmount()returns the layer's reserved mount point.lib/internal/vfs/setup.jsfindVFS(O(1) lookup), fs handler, loader overrides with parity tosrc/node_modules.cc/src/node_file.cc, prefix-scan cache purge.lib/internal/modules/helpers.jsloader*wrappers,setLoaderFsOverrides/setLoaderPackageOverrides,purgeRealpathCacheForPrefix.lib/internal/modules/cjs/loader.jsstat()+ TS read routed through wrappers;purgeModuleCachesForPrefixfor unmount.lib/internal/modules/esm/resolve.jslegacyMainResolve+internalModuleStat+toRealPathrouted. No URL decoration.lib/internal/modules/esm/load.jsgetSourceSyncreads via the wrapper.lib/internal/modules/esm/get_format.jslib/internal/modules/package_json_reader.jspurgePackageJSONCacheForPrefix.lib/fs.jsstatSync/lstatSynchonourthrowIfNoEntry:falseon ENOENT from the VFS handler.doc/api/vfs.mdTests (all gated by
--experimental-vfs):test-vfs-mount,test-vfs-mount-errors,test-vfs-multi-mount,test-vfs-require,test-vfs-import,test-vfs-module-hooks,test-vfs-module-hooks-cleanup,test-vfs-package-json,test-vfs-package-json-cache,test-vfs-invalid-package-json,test-vfs-scoped-cache-purge,test-vfs-layer-id,test-vfs-layer-tag-prefix.Refs
The reserved-namespace design follows the "no interference with valid paths in the file system" requirement from the SEA VFS requirements doc.
Out of scope
SEA + VFS, overlay/stacking of multiple VFS layers under one prefix, migrating the C++
package_configs_cache, broader permission-model integration.